Repository navigation
ENH: add remfile backend with pluggable backend architecture - #131
yarikoptic wants to merge 10 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #131 +/- ##
==========================================
+ Coverage 92.84% 97.04% +4.20%
==========================================
Files 13 18 +5
Lines 1188 2100 +912
==========================================
+ Hits 1103 2038 +935
+ Misses 85 62 -23 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Refactor what used to live in `datalad_fuse/fsspec.py` into a small pluggable-backend layer so datalad-fuse can serve file content from backends other than fsspec — starting with *remfile*, which offers chunk-based HTTP access optimised for HDF5-structured files (.nwb, .h5, .hdf5, .hdf, .he5, .nc, .nc4). Module layout ------------- - datalad_fuse/backends.py — Backend ABC + DEFAULT_BACKENDS. - datalad_fuse/remfile.py — RemfileBackend + RemfileWrapper. Lazy remfile import, EXTENSIONS frozenset on the class. - datalad_fuse/adapter.py — RemoteFilesystemAdapter (renamed from FsspecAdapter), DatasetAdapter, FileState, resolve_backends, create_backends, is_http_url. - datalad_fuse/fsspec.py — slimmed to FsspecBackend + aiohttp helpers, with a module-level __getattr__ emitting DeprecationWarning for the moved names so downstream imports keep working. - datalad_fuse/fuse_.py, fsspec_head.py, fsspec_cache_clear.py, __init__.py — updated to use the new adapter and expose `--backends` (comma-separated priority list). Backend semantics ----------------- - Backend ABC: `name`, `can_handle(key, mode)`, `open_url(...)`, `clear()`. - DatasetAdapter.open walks the backend chain: tries each URL of the first backend that can_handle the key, falls through to the next backend on any exception, and finally raises IOError chained to the last cause if no backend succeeded. - resolve_backends returns (spec, explicit); explicit=True for user/config spec, False for the default. create_backends logs a warning (not debug) when an explicit backend is missing. Build / test infrastructure tie-ins ----------------------------------- - setup.cfg: add `remfile` and `full` extras_require. - tox.ini: add a `py314-full` env (via factor conditionals) that installs the `full` extra and passes `--libfuse --network` to pytest. Register `network` + `ai_generated` markers. Add the new source modules to the typing env mypy invocation. - .github/workflows/test-libfuse.yml: route Python 3.14 through `py314-full` so remfile + network tests exercise on one runner. Co-Authored-By: Claude Code 2.1.114 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- test_backends.py: unit tests for can_handle dispatch, backend-chain fallback in DatasetAdapter.open (first-wins, fall-through on failure, skip-on-cannot-handle, IOError with __cause__), RemoteFilesystemAdapter lifecycle (__exit__ closes datasets), resolve_backends precedence (argument > config > default), create_backends (unknown name, unavailable silent vs. warned, all-unavailable raises), FsspecBackend BlocksizeMismatchError retry, RemfileWrapper (read/seek/iter/info with and without Content-Length), is_http_url parametrised, ABC-compliance check across backends, and integration tests against dandiarchive S3 (marked `network`). Optional-dependency gating uses a shared `requires_remfile` pytest.mark.skipif. - test_deprecations.py: verifies the old `datalad_fuse.fsspec.*` import paths still resolve to the new locations and emit DeprecationWarning; verifies FsspecBackend (canonical) imports without warning; verifies unknown names raise AttributeError (direct access) / ImportError (from-import). - conftest.py: add `--network` CLI opt-in mirroring `--libfuse`. Co-Authored-By: Claude Code 2.1.114 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a pluggable remote-file backend architecture (defaulting to remfile,fsspec) and introduces a remfile backend optimized for HDF5-style access while preserving legacy datalad_fuse.fsspec import paths via DeprecationWarning.
Changes:
- Introduces backend ABC + backend resolution/creation logic and refactors the adapter layer into
adapter.py - Adds
RemfileBackend+ wrapper, and refactorsfsspec.pyinto a backend + compatibility shim - Updates CLI interfaces and tests to support
--backendsand validate backward compatibility
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tox.ini | Installs additional extras and expands mypy targets to cover new modules |
| setup.cfg | Adds new extras (remfile, full) for optional backend dependencies |
| datalad_fuse/tests/test_forgejo.py | Updates imports to the new module locations |
| datalad_fuse/tests/test_deprecations.py | Adds tests for legacy import-path deprecation behavior |
| datalad_fuse/tests/test_backends.py | Adds unit/integration tests for backend chaining, remfile, and fsspec behavior |
| datalad_fuse/remfile.py | Implements RemfileBackend and RemfileWrapper |
| datalad_fuse/fuse_.py | Switches FUSE operations layer to use RemoteFilesystemAdapter + adds backend option |
| datalad_fuse/fsspec_head.py | Adds --backends CLI option and switches to new adapter |
| datalad_fuse/fsspec_cache_clear.py | Updates imports to the new adapter module |
| datalad_fuse/fsspec.py | Refactors into FsspecBackend, keeps async HTTP helpers, adds compatibility __getattr__ |
| datalad_fuse/backends.py | Adds Backend ABC and DEFAULT_BACKENDS |
| datalad_fuse/adapter.py | Adds backend-agnostic dataset/remote filesystem adapter and backend chain logic |
| datalad_fuse/init.py | Adds --backends option to the main FuseFS interface |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- FsspecBackend.open_url: re-raise BlocksizeMismatchError when caching is
disabled instead of calling pop_from_cache (which would AttributeError on
a plain HTTPFileSystem). Update existing retry test to use caching=True
and add a propagation test for caching=False.
- create_backends: silently skip empty entries from spec.split(",") so
inputs like "remfile,,fsspec" or "fsspec," are tolerated rather than
raising "Unknown backend: ''". Add parametrized test.
Spotted by Copilot review on PR datalad#131.
Co-Authored-By: Claude Code 2.1.132 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- tox.ini: scope the `full` extra to envs with `full` or `libfuse` factor
rather than installing it in every environment. remfile is meant to be
optional, and the `requires_remfile` skipif on the tests already handles
the case when it is not installed, so the default
`py3{10..14}-nonetwork` rows can stay lean.
- remfile.py: cap RemfileWrapper.__next__ at _MAX_LINE_BYTES (1 MiB) so
iterating a binary HDF5/NWB file (no '\n') does not download the entire
remote object in a single iteration step. Add test_iteration_caps_no_newline
exercising the cap with a shrunk limit.
Spotted by Copilot review on PR datalad#131.
Co-Authored-By: Claude Code 2.1.132 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
RemfileBackend.open_url now raises NotImplementedError when mode != 'rb'. can_handle() already returns False for text modes, so the backend chain will never call open_url() with such a mode in practice — but if a caller bypasses can_handle() we want a loud failure rather than a silently-binary handle. Add parametrized test_open_url_rejects_non_binary_mode covering 'r', 'rt', 'w', 'wb'. Spotted by Copilot review on PR datalad#131. Co-Authored-By: Claude Code 2.1.132 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pulls in datalad#133 "ENH: add S3 exporttree URL fallback for legacy datasets" (790ff48) plus its CHANGELOG entry and dependency addition. Conflict resolution: - datalad_fuse/fsspec.py: kept this branch's slim refactored module (FsspecBackend + HTTP helpers + deprecation `__getattr__`). Master added 4 methods to a DatasetAdapter still living in fsspec.py; those methods are ported to datalad_fuse/adapter.py where DatasetAdapter now lives. - datalad_fuse/adapter.py: added * `subprocess`, `boto3`, `botocore.UNSIGNED`, `BotocoreConfig`, `itertools.chain` imports * `DatasetAdapter._get_exporttree_remotes` (cached, parses git-annex:remote.log) * `DatasetAdapter._list_s3_versions` (anonymous boto3 client) * `DatasetAdapter._match_s3_version` (size + ETag disambiguation) * `DatasetAdapter.get_exporttree_urls` (versioned S3 URLs with unversioned fallback) In `DatasetAdapter.open`, the per-backend URL loop now chains `get_urls(str(key))` with `get_exporttree_urls(relpath, key)` so the S3 fallback is tried after the primary URLs for every URL-capable backend (fsspec, remfile). Lazy: boto3 calls only happen when the primary URL set is exhausted. - datalad_fuse/tests/test_exporttree.py: updated import to `from datalad_fuse.adapter import DatasetAdapter` (canonical path); the deprecation shim still exposes it from `.fsspec` but the test shouldn't trigger DeprecationWarning on every run. - CHANGELOG.md, docs/designs/20260408-s3-exporttree-url-fallback.md, setup.cfg (boto3 added to install_requires): taken from master unchanged. Co-Authored-By: Claude Code 2.1.132 / Claude Opus 4.7 (1M context) <noreply@anthropic.com>
yarikoptic-gitmate
left a comment
There was a problem hiding this comment.
Review and test drive of 3c3bdd8
The backend chain and the module split look good to me. The fsspec.py shim works, CI is green, and in a real FUSE mount remfile reads HDF5 noticeably faster. I found two real bugs in RemfileWrapper that I'd fix before merging, and two behaviour changes in the new default (remfile,fsspec) that deserve a decision or documentation. Details are in the inline comments; summary below.
Should fix before merge
RemfileWrapper.info()makes a fresh HEAD request. FUSE reaches it viagetattr(path, fh)on open handles. Against a server that rejects HEAD (which presigned S3 GET URLs do),fstaton an open file fails withHTTPError 403; fsspec gets the size fine. The fix needs no network call:self._f.length.RemfileWrapper.read(): with no size it returnsb""and moves the position back by 1. It never clamps at EOF. For files whose size is a multiple of remfile's 100 KiB chunk, a read at EOF raises416 Range Not Satisfiable, which also breaks line iteration and sofsspec-head -n. The suggested clamp fixes all three. Also minor:seek()returnsNone.
Behaviour changes to decide on or document
- Dead URLs stall about 30 s each under remfile (its open probe retries 8× with backoff), versus about 1 s under fsspec. I measured 30.2 s vs 1.07 s on dandiset 000027. In
fusefsthis happens under the globalrwlock, so the whole mount stalls. --caching=ondiskno longer applies to HDF5 files once remfile is installed: no cache files were written, andfsspec-cache-clearhas nothing to clear for them.
Smaller points
- URL/VURL keys (
addurl --fast/--relaxed) have no suffix, so.nwbfiles with such keys go to fsspec. Falling back torelpath's extension would fix that. datalad.fusefs.backendsis read from the globalcfg, so dataset-local config is ignored. Same as the existingcache-clearoption, butds.configis right there.- When a backend falls through,
get_urls()and the exporttree lookup (whereissubprocess, boto3) run again. - The README isn't updated: nothing on
--backends,datalad.fusefs.backends, or the[remfile]/[full]extras, and the intro still says fsspec is used. The--cachinghelp says "fsspec'ed". - Nit: with
--backends remfileonREADME.md, the error isCould not open README.md within … (backends=remfile)with no cause attached. Saying that no configured backend handles.mdwould be clearer.
What I ran
Setup: Ubuntu 24.04, Python 3.13, datalad 1.7.1, git-annex 10.20260901, remfile 0.1.15, fsspec 2026.9.0, h5py 3.16, libfuse2.
- Unit and network tests:
test_backends.py test_deprecations.py test_exporttree.py test_util.py→ 134 passed, including the S3 integration tests. - Full suite with
--libfuse: 211 passed. The rest is environmental: 8test_forgejoerrors (no docker in my sandbox) and 1 failure oftest_fuse.py::test_parallel_access, because archive.org is blocked by the sandbox network policy. That test fails the same way onmaster. - flake8 and mypy (the tox
lint/typinginvocations): clean. - End to end, own dataset:
MD5E.nwb/.hdf5/.mdfiles registered to the pinneddandiarchive.s3objects, then dropped.fsspec-headdispatched correctly: remfile for.nwb/.hdf5, fsspec for.md.--backends fsspec|remfile|bogus|' , 'and-c datalad.fusefs.backends=fsspecall behaved as described.- Through
fusefs, thesha256sumof all three files matched the originals, and h5py walked both HDF5 files (42 and 5 objects).
- Real dandiset 000004, a 73 MB
ecephys+image.nwbread throughfusefs:- Full sequential read: 4.6 s with remfile vs 22.6 s with fsspec. The checksum matched the
SHA256Ekey in both cases. - h5py open, traverse and a small slice: comparable and noisy (≈5.2 s vs 3.1–6.9 s across runs).
- Peak RSS: similar (~146 vs ~141 MiB).
- Full sequential read: 4.6 s with remfile vs 22.6 s with fsspec. The checksum matched the
- Deprecation shim: old names warn, unknown names still raise
AttributeError/ImportError, andFsspecBackendimports without a warning.
Generated by Claude Code
| # caller still makes progress instead of OOM-ing on a binary file. | ||
| return b"".join(chunks) | ||
|
|
||
| def info(self) -> dict[str, Any]: |
There was a problem hiding this comment.
Bug: the HEAD request breaks fstat() on open FUSE handles when the server rejects HEAD.
This is not "rarely called". DataLadFUSE.getattr(path, fh) takes the "File already open" branch and calls file_getattr(f) → f.info() whenever the kernel asks for attributes of an open handle (fgetattr). I saw this in a real mount log while h5py was opening ros3test.nwb. It does not depend on the key carrying a size.
The HEAD is also the request remfile itself avoids, because presigned S3 GET URLs (e.g. what DANDI's api.dandiarchive.org/.../download/ redirects to) reject HEAD. When that happens, urllib raises HTTPError, which is not a FileNotFoundError, so file_getattr does not map it to ENOENT and fusepy gets an unhandled exception.
Reproduced deterministically: a dataset whose only URL is a local server that answers HEAD with 403 and serves ranged GETs. I called ops = DataLadFUSE(ds, caching=False, backends=...), then fh = ops.open(path, O_RDONLY), then ops.getattr(path, fh):
== remfile,fsspec
open -> 1000 RemfileWrapper
getattr(path, fh) FAILED: HTTPError HTTP Error 403: Forbidden
== fsspec
open -> 1000 HTTPFile
getattr(path, fh) size: 177728
fsspec survives because HTTPFile.info() returns the details it cached at open time. remfile also already knows the size, since RemFile.__init__ fetched Content-Length with an aborted GET. So no network call is needed here:
| def info(self) -> dict[str, Any]: | |
| def info(self) -> dict[str, Any]: | |
| """Minimal info dict matching the fsspec convention. | |
| remfile already determined the size (via an aborted GET, which unlike | |
| HEAD also works for presigned URLs) when the file was opened. | |
| """ | |
| return {"type": "file", "size": self._f.length} |
With this change, the urllib.request import and the two test_info_* tests that mock urlopen need adjusting.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in d7fb6b6 BF: address review of PR #131 — RemfileWrapper semantics, URL probing, caching. info() now returns {"type": "file", "size": self._f.length} and makes no request at all. The urllib.request import stays in the module, but only for the new size probe in open_url() (see the reply on the retry/stall thread). The two test_info_* tests that mocked urlopen are replaced by test_info_uses_remfile_length, which asserts urlopen is not called.
| def read(self, size: int = -1) -> bytes: | ||
| return self._f.read(size) # type: ignore[no-any-return] |
There was a problem hiding this comment.
Bug: read() with no size returns b"" and moves the position backwards, and reading at EOF can raise.
RemFile.read() does not support size=-1. With -1, its chunk range comes out empty, so it returns b"" and then does self._position += size. Against the real S3 object:
f.seek(10); f.read() -> 0 bytes; tell() == 9
fsspec: g.seek(10); g.read() -> 177718 bytes
RemoteFilesystemAdapter.open() returns this object typed as IO, so any API user calling .read() silently gets empty data.
RemFile.read(n) has two related problems:
- It always advances by
n, even past EOF.read(100)atlength-5leavestell() == length+95. - When the size is an exact multiple of its 100 KiB chunk, a read at EOF issues an unsatisfiable Range request instead of returning
b"". That breaks__next__'s EOF detection, sofsspec-head -nfails. On a 204800-byte.h5served locally:
iteration FAILED at EOF: _PermanentHTTPError Error fetching bytes 204800-204799: 416 Range Not Satisfiable
Clamping to the remaining length fixes all of these:
| def read(self, size: int = -1) -> bytes: | |
| return self._f.read(size) # type: ignore[no-any-return] | |
| def read(self, size: Optional[int] = -1) -> bytes: | |
| # remfile requires an explicit size, does not clamp at EOF, and may | |
| # issue an unsatisfiable Range request when reading at EOF | |
| remaining = max(self._f.length - self._f.tell(), 0) | |
| if size is None or size < 0 or size > remaining: | |
| size = remaining | |
| if not size: | |
| return b"" | |
| return self._f.read(size) # type: ignore[no-any-return] |
Generated by Claude Code
There was a problem hiding this comment.
Fixed in d7fb6b6 BF: address review of PR #131 — RemfileWrapper semantics, URL probing, caching, with the suggested clamp. __next__ now reads through self.read() rather than self._f.read(), so line iteration gets the clamp too — otherwise the 416 at EOF still broke fsspec-head -n.
The mock in _make_mock_remfile was made faithful to remfile.RemFile rather than well-behaved (requires an explicit non-negative size, advances past EOF, raises 416 at EOF, seek() returns None); with that, the existing test_iteration/test_iteration_empty went red before the fix. Checked against the real S3 object too: seek(size); read(10) → b"", over-long read returns the tail and leaves tell() at EOF, read() with no size returns all 4008 bytes.
| def seek(self, offset: int, whence: int = 0) -> int: | ||
| return self._f.seek(offset, whence) # type: ignore[no-any-return] |
There was a problem hiding this comment.
Minor: RemFile.seek() has no return statement, so this returns None despite -> int. mypy doesn't catch it because self._f is Any. In the test drive, f.seek(100) gave None, while io.BytesIO(...).seek(100) gives 100.
| def seek(self, offset: int, whence: int = 0) -> int: | |
| return self._f.seek(offset, whence) # type: ignore[no-any-return] | |
| def seek(self, offset: int, whence: int = 0) -> int: | |
| self._f.seek(offset, whence) | |
| return self.tell() |
Generated by Claude Code
There was a problem hiding this comment.
Fixed in d7fb6b6 BF: address review of PR #131 — RemfileWrapper semantics, URL probing, caching, as suggested. Covered by test_seek_returns_new_position.
| raise NotImplementedError( | ||
| f"RemfileBackend only supports mode='rb', got {mode!r}" | ||
| ) | ||
| return cast(IO, RemfileWrapper(self._remfile.File(url), url)) |
There was a problem hiding this comment.
Unreachable URLs stall about 30× longer than with fsspec, and in FUSE that blocks the whole mount.
remfile.File(url) probes Content-Length and retries transient failures (connection errors, 5xx) 8 times with exponential backoff, about 25 s plus connect time. Only 4xx fails fast. The adapter tries URLs one after another, so each dead URL costs that much before the next one is tried.
Test: dandiset 000027. Its first whereis URL is api.dandiarchive.org/..., which my sandbox's network policy blocks. Its second URL is a versioned dandiarchive.s3 one. Running datalad fsspec-head -c 8 sub-RAT123/sub-RAT123.nwb:
--backends |
wall time |
|---|---|
remfile,fsspec (default) |
30.2 s |
fsspec |
1.07 s |
In fusefs, DataLadFUSE.open() calls self._adapter.open() while holding the global rwlock, so every other read on the mount waits the whole time. Dead mirrors, firewalled hosts, offline special-remote URLs and the several candidate URLs that get_urls() builds per HTTP remote all hit this.
Possible fix: do a cheap probe here with no retries and a short timeout, for example a ranged GET bytes=0-0 that also reads Content-Range to get the size, so URL fall-through stays fast. Then pass the known size to remfile (key.size is almost always available) so it skips its own probe. remfile only exposes that as the underscore-private _size=, though. It may be worth asking remfile upstream for public size= and retry parameters, or pinning the remfile version.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in d7fb6b6 BF: address review of PR #131 — RemfileWrapper semantics, URL probing, caching, along the lines you suggested. RemfileBackend.open_url() now does the cheap probe itself — one GET bytes=0-0 with a 10 s timeout and no retries — reads the total from Content-Range (falling back to Content-Length when the server ignores Range, and to None when it says neither), and passes it to remfile.File(..., _size=...) so remfile skips its own retrying probe. For a live URL that is the same single round trip it was already paying.
Measured against a missing object in the same bucket: 0.20 s, down from ~30 s. The pinned-S3 integration tests still pass unchanged.
_size= is private API, so setup.cfg now pins remfile ~= 0.1.15. Asking upstream for a public size= (and retry parameters) still seems worth doing — happy to file that separately.
| def can_handle(self, key: Optional[AnnexKey], mode: str) -> bool: | ||
| if mode != "rb": | ||
| return False | ||
| if key is None or key.suffix is None: | ||
| return False | ||
| return key.suffix.lower() in self.EXTENSIONS |
There was a problem hiding this comment.
Dispatching only on key.suffix misses HDF5 files whose keys come from a non-*E backend. git annex addurl --fast/--relaxed (and datalad addurls --fast) produce URL/VURL keys, and AnnexKey.parse sets suffix=None for those:
fast.nwb VURL-s177728--https://dandiarchive.s3.amazonaws.com/ros3test.nwb
relaxed.hdf5 VURL--https://dandiarchive.s3.amazonaws.com/ros3test.hdf5
fast.nwb: backend remfile cannot handle (suffix=None, mode=rb)
fast.nwb: opening via backend fsspec
The dandisets/* datasets use SHA256E, so they are fine. DatasetAdapter.open() already has relpath, though. Passing it to can_handle() (e.g. can_handle(key, mode, relpath)) and falling back to PurePosixPath(relpath).suffix when the key has no suffix would make the choice follow what the user sees in the tree.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in d7fb6b6 BF: address review of PR #131 — RemfileWrapper semantics, URL probing, caching. Backend.can_handle() takes a third relpath argument (optional, so the signature stays usable standalone), and RemfileBackend falls back to PurePosixPath(relpath).suffix when the key carries no suffix. DatasetAdapter.open() passes the relpath it already has. A key suffix still wins when present.
| if name == "fsspec": | ||
| backends.append(FsspecBackend(path, caching)) | ||
| elif name == "remfile": | ||
| backends.append(RemfileBackend()) |
There was a problem hiding this comment.
--caching=ondisk is now silently a no-op for HDF5 files once remfile is installed. caching is passed only to FsspecBackend. With the new default remfile,fsspec, .nwb/.h5 files bypass the on-disk cache entirely:
datalad fsspec-head --caching ondisk --backends remfile,fsspec -c 8 ros3test.nwb -> 0 files in .git/datalad/cache
datalad fsspec-head --caching ondisk --backends fsspec -c 8 ros3test.nwb -> 2 files in .git/datalad/cache
So installing datalad-fuse[full] quietly changes what --caching ondisk does for exactly the large files where it matters most. Options:
- wire
remfile.DiskCacheinto e.g..git/datalad/cache/remfile, and clear it inRemfileBackend.clear()sofsspec-cache-clearanddatalad.fusefs.cache-clearkeep working; - have
RemfileBackenddecline whencaching=True; - at least document it, and update the
--cachinghelp, which still says "cache fsspec'ed files".
Generated by Claude Code
There was a problem hiding this comment.
Fixed in d7fb6b6 BF: address review of PR #131 — RemfileWrapper semantics, URL probing, caching — the first of your three options. RemfileBackend now takes (path, caching) like FsspecBackend, builds a remfile.DiskCache at .git/datalad/cache/remfile when caching is on, passes it to remfile.File(..., disk_cache=...), and removes the directory in clear(), so fsspec-cache-clear and datalad.fusefs.cache-clear cover HDF5 files again. Verified end to end against the real S3 object: a 100-byte read writes a chunk under that directory, and clear() removes it.
The --caching help text still says "fsspeced" — that and the README are in the follow-up noted on the PR.
| """ | ||
| if backends is not None: | ||
| return backends, True | ||
| from_cfg = cfg.get("datalad.fusefs.backends", None) |
There was a problem hiding this comment.
The global datalad.cfg doesn't read the dataset's own .git/config or .datalad/config. So git config datalad.fusefs.backends fsspec inside a dataset is ignored, whether the command runs from inside it or with -d. I checked both: remfile was still used. This matches how datalad.fusefs.cache-clear is read today, so it is not a regression. DatasetAdapter.__init__ already builds Dataset(path), though, so ds.config.get("datalad.fusefs.backends") would honor per-(sub)dataset settings at no extra cost.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in d7fb6b6 BF: address review of PR #131 — RemfileWrapper semantics, URL probing, caching. resolve_backends() takes an optional config, and DatasetAdapter.__init__ passes ds.config. That is a superset of the global one rather than a replacement: ConfigManager.__init__ does self.overrides = datalad.cfg.overrides.copy(), so -c datalad.fusefs.backends=... still wins. The global cfg remains the fallback when no config is supplied.
I left datalad.fusefs.cache-clear alone, since that is outside this PR.
| primary_urls = self.get_urls(str(key)) | ||
| fallback_urls: Iterator[str] = ( | ||
| self.get_exporttree_urls(relpath, key) | ||
| if key is not None | ||
| else iter([]) | ||
| ) | ||
| for url in chain(primary_urls, fallback_urls): |
There was a problem hiding this comment.
Minor (efficiency): when the first backend exhausts its URLs, the next backend calls get_urls() and get_exporttree_urls() again. That repeats the git annex whereis subprocess (batch=False), the two examinekey calls and, on the exporttree path, the boto3 list_object_versions call. Collecting the URLs lazily into a list the first time and replaying it for later backends would avoid that, and the lazy boto3 behaviour would stay the same.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in d7fb6b6 BF: address review of PR #131 — RemfileWrapper semantics, URL probing, caching. Added a small replayable() helper: the chained get_urls() + get_exporttree_urls() iterator is pulled at most once, and each backend replays what has already been seen before pulling anything new — so the laziness of the boto3 call is unchanged, but whereis/examinekey/list_object_versions run once per open() rather than once per backend. test_urls_enumerated_once_across_backends pins the call count; test_urls_not_enumerated_when_no_backend_handles pins the laziness.
…probing, caching Six points from the review of 3c3bdd8: * RemfileWrapper.info() issued a HEAD request. FUSE reaches it through DataLadFUSE.getattr(path, fh) on every fstat() of an open handle, so against a server that rejects HEAD -- which presigned S3 GET URLs do -- fstat() failed with HTTPError. remfile already knows the size, so report self._f.length and make no request at all. * RemfileWrapper.read() delegated straight to remfile, which requires an explicit non-negative size: read() with no size returned b"" and moved the position back by one, reads never clamped at EOF, and a read at EOF issued an unsatisfiable Range request (416), breaking line iteration and hence fsspec-head -n. Clamp to the remaining length, and route __next__ through the wrapper so it sees the clamp too. * RemfileWrapper.seek() returned None despite declaring -> int. * RemfileBackend.open_url() now probes the size itself with one ranged GET bytes=0-0 on a short timeout and hands it to remfile via _size=. remfile's own probe retries 8x with backoff, so a dead URL cost ~30 s before the chain moved on -- under the global rwlock in fusefs, stalling the whole mount. Measured against a missing S3 object: 0.20 s, down from ~30 s. setup.cfg pins remfile ~= 0.1.15, since _size= is private API. * --caching=ondisk was silently a no-op for .nwb/.h5 files once remfile was installed. RemfileBackend now takes (path, caching) like FsspecBackend and wires a remfile.DiskCache at .git/datalad/cache/remfile, cleared by clear(), so fsspec-cache-clear and datalad.fusefs.cache-clear keep working. * can_handle() takes the dataset-relative path and falls back to its suffix when the annex key has none, so .nwb files with URL/VURL keys (addurl --fast/--relaxed) reach remfile. * datalad.fusefs.backends is read from the dataset's own config, which inherits the global overrides, so per-(sub)dataset settings are honored. * DatasetAdapter.open() enumerates URLs once and replays them for later backends, instead of re-running whereis, examinekey and the boto3 list_object_versions call per backend. Still open, deliberately left for a follow-up: the README says nothing about --backends, datalad.fusefs.backends or the [remfile]/[full] extras, the --caching help still says "fsspec'ed", and the "Could not open" error does not say when no configured backend handles the file. Co-Authored-By: Claude Code 2.1.292 / Claude Opus 5 (1M context) <noreply@anthropic.com>
Conflict in datalad_fuse/fsspec.py: master added docstrings to FileState, DatasetAdapter and FsspecAdapter, all of which this branch moved to datalad_fuse/adapter.py (with FsspecAdapter renamed RemoteFilesystemAdapter). Resolved by keeping this branch's slim fsspec.py and porting the docstrings into adapter.py, extended for the backend chain: the `backends` parameter, the per-backend cache directories, and what open() now returns and raises. The DataLadFUSE docstring master added in fuse_.py referred to datalad_fuse.fsspec.FsspecAdapter / .DatasetAdapter; repointed at datalad_fuse.adapter and documented its `backends` parameter. The .rst documentation master added still describes the pre-split module layout and says nothing about backends; that is the next commit. Co-Authored-By: Claude Code 2.1.292 / Claude Opus 5 (1M context) <noreply@anthropic.com>
master's documentation rework landed while this branch split fsspec.py into adapter.py / backends.py / remfile.py and added the pluggable backends, so the .rst pages still described the pre-split module layout and did not mention backends at all. Module layout, everywhere it is referenced: - datalad_fuse.fsspec -> datalad_fuse.adapter, FsspecAdapter -> RemoteFilesystemAdapter, in README.md, index, concepts, python, tutorial, troubleshooting and conf.py's lru_cache unwrapping; - python.rst and api.rst note that the old names still work with a DeprecationWarning. New material: - concepts.rst: a "Backends" section (anchor concepts-backends) comparing remfile and fsspec -- what each handles, what it fetches, how it is installed -- how the chain is configured, and how a file is dispatched (key suffix, falling back to the path's); the caching section now covers the per-backend cache directories and remfile's lack of an expiry; the failure message is the current "Could not open <path> within <dataset> (backends=...)". - installation.rst: an "Optional backends" section for the [remfile] and [full] extras (anchor installation-backends). - cli.rst: --backends on fusefs and fsspec-head, and a section on the datalad.fusefs.backends configuration option. - python.rst: the adapters' `backends` argument, and what open() now returns per backend. - api.rst: datalad_fuse.backends / .remfile / FsspecBackend, plus resolve_backends and create_backends. - troubleshooting.rst: the backend-related warnings and errors, how to see which backend took a file, and remfile's chunk size under "Reading is slow". - tutorial.rst: installs datalad-fuse[remfile], since it is all NWB. - CONTRIBUTING.md: which tox envs have remfile. Also docstrings that the documentation renders: short ones for RemfileWrapper.read/seek/tell/close, a `#:` doc for DEFAULT_BACKENDS, and two cross-references that did not resolve because the role was split across lines. `make -C docs html` (warnings are errors) is clean, and a nitpicky build reports nothing new. Co-Authored-By: Claude Code 2.1.292 / Claude Opus 5 (1M context) <noreply@anthropic.com>
It is not only fsspec that fetches the data any more, and `ondisk` now also covers what the remfile backend caches. Co-Authored-By: Claude Code 2.1.292 / Claude Opus 5 (1M context) <noreply@anthropic.com>
Documentation build overview
23 files changed ·
|
With datalad/datalad-fuse#131 and remfile installed, remfile reads HDF5 files, with its own read-ahead (chunks growing up to 100 MiB) that datalad-fuse does not let us configure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
…-access Conflicts with datalad#131 (pluggable backends), on which this branch is based, resolved by keeping master's wording where it still holds, and the backend-specific content of datalad#131 otherwise: - `--caching` help: master's wording, plus datalad#131's `--backends` parameter. - tox.ini: master's `FUSE_LIBRARY_NAME` passenv, plus datalad#131's `full` extra. - fsspec.py: datalad#131's (its `DatasetAdapter` moved to adapter.py, whose `caching` docstring gets master's "a week after they were first cached"). - docs: master's trimmed index and troubleshooting list (with the adapter classes renamed), datalad#131's error message and per-backend caches in the concepts, performance and tutorial sections; `_hide_enum_signature` in conf.py now names `datalad_fuse.adapter.FileState`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
yarikoptic
left a comment
There was a problem hiding this comment.
Follow-up: corrections to my review of 3c3bdd8, and a re-check of b4dbee6
Posted from @yarikoptic's account by Claude Code; the earlier review came from @yarikoptic-gitmate.
After posting the earlier review, I had it fact-checked by independent re-runs. Some statements in it were wrong or overstated; they are corrected at the end, for the record. Most of the underlying problems are already fixed in d7fb6b6. I re-tested them on b4dbee6.
Verified fixed on b4dbee6
datalad fsspec-head ros3test.hdf5returns all 4008 bytes. The line iterator had silently truncated it to 6 bytes; going through the clampedread()fixed that.-c 300000 ros3test.nwbreturns 177728 bytes in about 2 s.- Dandiset 000027, whose first URL is unreachable from my sandbox: 2.9 s with the default backends, down from 30.2 s.
info()makes no request.
Still open
--caching ondiskcan serve corrupted data. An entry in remfile'sDiskCachethat was cut short (e.g. by an interrupted write) is returned as file data. Details, a verified reproduction and a tested wrapper are in the inline comment.fsspec-cache-clearonly clears the caches of the configured backends. It buildsDatasetAdapter(ds.path, caching=True), whose backends come fromdatalad.fusefs.backends. With that set tofsspec, it reportsokbut leaves.git/datalad/cache/remfilein place (verified on b4dbee6); withremfile, the fsspec cache stays. Clearing both cache directories regardless of the backend setting would match what the command's name promises.- remfile sends its range requests without a timeout. The new probe has a 10 s timeout, but the reads that follow have none. So a server that stops answering mid-read blocks that FUSE
read()forever, while it holds the globalrwlock. The proper fix is in remfile, to be reported there. Until then, therequests.Sessionthat remfile creates could be given a default timeout. - Pre-existing, not from this PR: fusepy 3.0.1 mishandles an
OSErrorwhoseerrnoisNone. Its error handler raisesTypeError, thenNameError, and the syscall fails with ERANGE. On 3c3bdd8 that is how the HEAD-requestHTTPErrorreached applications.requests' exceptions are alsoOSErrors witherrnoNone, so I expect a network error during a remfile read to take the same path; that is inferred from fusepy's code, not exercised. Catching backend errors inDataLadFUSE.open()/read()and raisingFuseOSError(EIO)would give applications a sensible error.
Corrections to my earlier review
info()thread:- The trigger was not
fstat(); fusepy passes no file handle for that. It was reads that cross EOF, andlseek(SEEK_END), more than ~1 s after the attributes were cached. - The effect was worse than an "unhandled exception": the syscall failed with ERANGE, so
catorsha256sumof such a file failed at EOF. - The d7fb6b6 commit message repeats the
fstat()wording. The fix is right either way.
- The trigger was not
read()thread:- On S3, a read into a chunk past EOF was not a fast 416 but a ~25 s retry loop. S3 answers the invalid
bytes=204800-177727with 200 and the whole object. - It happened for any read that reached a chunk lying entirely past EOF, not only for chunk-aligned sizes.
- My suggested patch alone was incomplete; d7fb6b6 already routes
__next__throughread().
- On S3, a read into a chunk past EOF was not a fast 416 but a ~25 s retry loop. S3 answers the invalid
seek()thread: mypy does flag theNonereturn (no-any-return); the code had silenced it with# type: ignore.- stall thread: "~30× slower" held only for connection-level failures. For a URL answering 5xx, fsspec was the slower one (~118 s vs ~25 s), and URLs that 404 failed fast in both.
- suffix thread: the cause is the key backend, not how the file was added. Keys from any backend whose name does not end in
E(MD5,SHA256,SHA1,WORM,URL,VURL) carry no suffix. Therelpathfallback in d7fb6b6 covers all of them. - Summary:
- "remfile reads HDF5 noticeably faster" was one noisy run and did not reproduce.
- Peak RSS was similar only for the 73 MB file. For a 600 MB file it was 774 MB with remfile vs 216 MB with fsspec.
- My test dataset used the unversioned S3 URLs, not "pinned" objects.
Separately, I have drafted reports for magland/remfile on the remfile-side bugs: reads past EOF, retries when a server ignores Range, missing timeouts, DiskCache integrity, the in-memory cache cap, and file-object behaviour. They will be filed there.
Generated by Claude Code
| size = self._probe_size(url) | ||
| # `_size` is private API; see the remfile pin in setup.cfg. | ||
| remfile_obj = self._remfile.File(url, _size=size, disk_cache=self._disk_cache) |
There was a problem hiding this comment.
--caching ondisk can now serve corrupted data. remfile.DiskCache writes each entry in place and get() returns whatever is on disk; remfile uses it without checking the length. So an entry cut short by an interrupted write is later served as file data. A killed fusefs process or a full disk can produce such an entry.
Verified on b4dbee6. After truncating one entry under .git/datalad/cache/remfile to half its size, datalad fsspec-head --caching ondisk -c 177728 ros3test.nwb returns 140064 bytes with a different checksum, and no error.
The proper fix belongs in remfile and will be reported there. In the meantime, the size that _probe_size() returns lets the backend reject damaged entries. remfile then fetches the chunk again and its set() rewrites the entry:
class _CheckedDiskCache:
"""Never return a remfile.DiskCache entry of the wrong length."""
def __init__(self, cache: Any, size: int) -> None:
self._cache = cache
self._size = size
def get(self, key: str) -> Optional[bytes]:
value = self._cache.get(key)
if value is not None:
# remfile's key is "<url>|<chunk size>|<chunk index>"
chunk_size, index = (int(x) for x in key.rsplit("|", 2)[1:])
if len(value) != min(chunk_size, self._size - index * chunk_size):
return None # damaged (e.g. interrupted write): fetch again
return value
def set(self, key: str, value: bytes) -> None:
self._cache.set(key, value) size = self._probe_size(url)
disk_cache = self._disk_cache
if disk_cache is not None and size is not None:
disk_cache = _CheckedDiskCache(disk_cache, size)
# `_size` is private API; see the remfile pin in setup.cfg.
remfile_obj = self._remfile.File(url, _size=size, disk_cache=disk_cache)I checked this directly against remfile 0.1.15 and the real S3 object, with both chunk entries truncated:
- plain
DiskCache: wrong data, 88864 bytes; - with the wrapper: correct data, 177728 bytes, and the entries were rewritten to 102400 and 75328 bytes;
- a third read was also correct.
Like the _size= call, this relies on remfile's 0.1.x key format, which the remfile ~= 0.1.15 pin covers.
Generated by Claude Code
…ckend Rather than dandi reading the keys and URLs of annexed files from git-annex and streaming them with remfile itself, the content is again streamed with datalad-fuse's adapter, now that it has a remfile backend and can be told not to initialize git-annex (datalad/datalad-fuse#131, and CodyCBakerPhD/datalad-fuse's remfile-dev branch, which CI installs until those are released): - The adapter (RemoteFilesystemAdapter, with caching=False and init_annex=False) gives the state and key of a file, the URLs git-annex knows for it, and opens it from the first URL that can be read, with remfile for HDF5-based files such as NWB. dandi only wraps the file object it returns as a raw binary file, for h5py. - git-annex being initialized is still checked first (`git config annex.uuid`), so that a DataLad dataset is not loaded for each broken symbolic link of a fresh clone. - The `streaming` extra is again a `datalad` extra, requiring datalad-fuse[remfile], and the CI job is named `datalad-fuse`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
…ackend Until datalad-fuse releases its remfile backend and the adapter's `init_annex` option (datalad/datalad-fuse#131), the `datalad` extra requires them from CodyCBakerPhD/datalad-fuse at a fixed commit, so that `pip install "dandi[datalad]"` gives working streaming, and CI installs just `.[datalad]`. This must be replaced by a release requirement before merging, as PyPI rejects distributions with direct URL dependencies. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
|
@yarikoptic Looking like this branch works well as it is for my purposes (and I'd prefer it over Can you resolve conflicts and maybe we can discuss possibility of merge? |
Pin the datalad extra to the head commit of datalad/datalad-fuse#131 rather than a fork combining it with an init_annex option of the adapter: dandi hands files to the adapter only in repositories in which git-annex is initialized, so the adapter never initializes it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr
Summary
--backendsCLI option (comma-separated, priority-ordered)remfilebackend (optionalpip install datalad-fuse[remfile]) optimized for HDF5 access patterns (.nwb, .h5, .hdf5, etc.)fsspec.pyinto focused modules:backends.py(ABC),remfile.py,adapter.py, slimmedfsspec.pydatalad_fuse.fsspecimport paths viaDeprecationWarningHow it works
The backend chain walks
--backends=remfile,fsspec(default) in order. Each backend'scan_handle(key, mode)decides if it should handle a file based on the annex key's extension. First match tries all URLs; on failure, falls through to the next backend. Configuration via--backendsflag,datalad.fusefs.backendsconfig variable, or programmatic API.New module layout
backends.pyBackendABC,DEFAULT_BACKENDSremfile.pyRemfileBackend,RemfileWrapperadapter.pyRemoteFilesystemAdapter(renamed fromFsspecAdapter),DatasetAdapter,FileStatefsspec.pyFsspecBackend, HTTP helpers, backward-compat__getattr__Commits
RF: address code review findingsTST: add backward compatibility deprecation testsENH: add remfile backend with pluggable backend architectureTest plan
pytest datalad_fuse/tests/test_backends.py datalad_fuse/tests/test_deprecations.py -k "not network"pytest datalad_fuse/tests/test_backends.py -k networkpytest datalad_fuse/tests/test_util.pyfrom datalad_fuse.fsspec import FsspecAdapterstill works with DeprecationWarning🤖 Generated with Claude Code
Overall it grew larger than initial quick "fix" was (first commit) since the whole setup had 'fsspec' naming throughout it.